netutils/ping: improve ping and ping6 for get_host error handling#3404
netutils/ping: improve ping and ping6 for get_host error handling#3404masc2008 wants to merge 1 commit intoapache:masterfrom
Conversation
dns look for both IPv4/v6 when do dns_query, so when do ping, it should try both ipv4 and ipv6 address before return "ping_gethostip" error. Signed-off-by: Jerry Ma <masc2008@gmail.com>
simbit18
requested review from
Donny9,
GUIDINGLI,
acassis,
anchao,
jerpelea,
linguini1,
michallenc,
raiden00pl and
xiaoxiang781216
| ****************************************************************************/ | ||
|
|
||
| static void ping6_result(FAR const struct ping6_result_s *result) | ||
| void ping6_result(FAR const struct ping_result_s *result) |
There was a problem hiding this comment.
why change to public function
| * Private Types | ||
| ****************************************************************************/ | ||
|
|
||
| struct ping6_priv_s |
There was a problem hiding this comment.
why move internal strut to header file
|
|
||
| struct ping6_result_s; | ||
|
|
||
| struct ping6_info_s |
There was a problem hiding this comment.
please move the reorg change to new pr
There was a problem hiding this comment.
The purpose is to let ping_result_s and ping6_result_s share same data structure. since in the callback same pointer was transferred, they will be used in different callback. same reason to ping_info_s and ping6_info_s.
if they have have private structure, and changed privately, then it will have very high risk, not easy to maintain them. considering this, should let them share same.
There was a problem hiding this comment.
The key change is here: others are for this:
There was a problem hiding this comment.
ping and ping6 is totally seperated in the old implementation, so I would suggest that:
- Keep the origin design, and change the above code to posix_spawn
- Implement ipv4/ipv6 ping in one file
either is better than the current change which couple ping/ping6 in adhoc way.
There was a problem hiding this comment.
I would say ipv4/ipv6 ping should be in one file, and that is correct way. if choose that direction: FreeBSD version of ping can match the requirement, so why do we need do such basic code again? what I did is some work with accept able cost and fully leverage the current file. if re-org based current ping code, it will a new folder with some repeated data structures. this task has afflicted me too long. I would rather give up...
There was a problem hiding this comment.
I would say ipv4/ipv6 ping should be in one file, and that is correct way. if choose that direction: FreeBSD version of ping can match the requirement, so why do we need do such basic code again? what I did is some work with accept able cost and fully leverage the current file. if re-org based current ping code, it will a new folder with some repeated data structures. this task has afflicted me too long. I would rather give up...
There was a problem hiding this comment.
I would say ipv4/ipv6 ping should be in one file, and that is correct way. if choose that direction: FreeBSD version of ping can match the requirement, so why do we need do such basic code again? what I did is some work with accept able cost and fully leverage the current file. if re-org based current ping code, it will a new folder with some repeated data structures.
There was a problem hiding this comment.
After further thinking, I still prefer my current direction.
First "posix_spawn" has two issues:
1. it still reports an extra "get host error" if just dns just replies one address
2. it will loop each endless if no dns replies if just do "posix_spawn" (This is a really bad problem).
Current ping/ping6 are quite well implemented, and just have a small bug. "Implement another ping" doesn't solve this bug in current ping tools.
It is allowed that several standalone tools share some common structures and functions. what I do is this such way, ping/ping6 are quite similar tools, and they should have some common things that can be leveraged by some public structures and functions. (considering that all referred functions are in "lib_ping_pub.c"). we still can configure the system to have either ping or ping6, they still goes same logic as before if just configured either one of them. If both ping and ping6 are configured it's a bug with those nuttx ping tools, we must have a solution to deal with such case.
Please consider my two cents. : )
3 participants
dns look for both IPv4/v6 when do dns_query, so when do ping, it should try both ipv4 and ipv6 address before return "ping_gethostip" error.
Note: Please adhere to Contributing Guidelines.
Summary
dns lookup for both IPv4/v6 when do dns_query, so when do ping, it should try both ipv4 and ipv6 address before return "ping_gethostip" error.
Impact
The network packet losing IPv4 or IPv6 dns resolve response is quite common in network pressure test. It will return get host error without this change. It's miss-leading report to end user. It's also quite common practice for linux to use one ping support both ping and ping6.
Testing
attach two network packet that packet losing for either v4 or v6.
Error log attached as below:
ping -c 5 www.baidu.com
ERROR: ping_gethostip(www.baidu.com) failed